Protect against concurrent access to Cargo.lock

CI tests seen to be spurious failing on OSX due to this, I believe it's because
one process is concurrently writing out Cargo.lock while the other is trying to
read it in, so one of them gets a corrupt version.

This would ideally be fixed from `pkg.root()` returning a `Filesystem`, but that
was unfortunately such an invasive change that it seemed untenable. Additionally
we generally don't write files into the source tree, so if this is the only
instance it's perhaps not so bad trying to be robust in to the future.

diff --git a/src/cargo/ops/cargo_generate_lockfile.rs b/src/cargo/ops/cargo_generate_lockfile.rs
index 1b8bb0087f0299e149d854198de73521c673298e..95056f2ce9d9fd458edc1b67446db2f0381e5743 100644 (file)
--- a/src/cargo/ops/cargo_generate_lockfile.rs
+++ b/src/cargo/ops/cargo_generate_lockfile.rs
@@ -23,7 +23,7 @@ pub fn generate_lockfile(manifest_path: &Path, config: &Config)
     let resolve = try!(ops::resolve_with_previous(&mut registry, &package,
                                                   Method::Everything,
                                                   None, None));
-    try!(ops::write_pkg_lockfile(&package, &resolve));
+    try!(ops::write_pkg_lockfile(&package, &resolve, config));
     Ok(())
 }
 
@@ -105,7 +105,7 @@ pub fn update_lockfile(manifest_path: &Path,
         }
     }
 
-    try!(ops::write_pkg_lockfile(&package, &resolve));
+    try!(ops::write_pkg_lockfile(&package, &resolve, opts.config));
     return Ok(());
 
     fn fill_with_deps<'a>(resolve: &'a Resolve, dep: &'a PackageId,

diff --git a/src/cargo/ops/cargo_pkgid.rs b/src/cargo/ops/cargo_pkgid.rs
index 2bf32e627f4802c1e29bfcc5c4865cbaf8e57787..6c6b1a88c0e45c9448feab37ae21aaddc5e9c31d 100644 (file)
--- a/src/cargo/ops/cargo_pkgid.rs
+++ b/src/cargo/ops/cargo_pkgid.rs
@@ -8,9 +8,7 @@ pub fn pkgid(manifest_path: &Path,
              spec: Option<&str>,
              config: &Config) -> CargoResult<PackageIdSpec> {
     let package = try!(Package::for_path(manifest_path, config));
-
-    let lockfile = package.root().join("Cargo.lock");
-    let resolve = match try!(ops::load_lockfile(&lockfile, &package, config)) {
+    let resolve = match try!(ops::load_pkg_lockfile(&package, config)) {
         Some(resolve) => resolve,
         None => bail!("a Cargo.lock must exist for this command"),
     };

diff --git a/src/cargo/ops/lockfile.rs b/src/cargo/ops/lockfile.rs
index e959203726f32d31e75a001f9ec743cb11c0fef4..ed0182d0c38dee06ee9d60ce6b6d3d3245f82500 100644 (file)
--- a/src/cargo/ops/lockfile.rs
+++ b/src/cargo/ops/lockfile.rs
@@ -1,45 +1,39 @@
-use std::fs::File;
 use std::io::prelude::*;
-use std::path::Path;
 
 use rustc_serialize::{Encodable, Decodable};
 use toml::{self, Encoder, Value};
 
 use core::{Resolve, resolver, Package};
-use util::{CargoResult, ChainError, human, paths, Config};
+use util::{CargoResult, ChainError, human, Config, Filesystem};
 use util::toml as cargo_toml;
 
 pub fn load_pkg_lockfile(pkg: &Package, config: &Config)
                          -> CargoResult<Option<Resolve>> {
-    let lockfile = pkg.root().join("Cargo.lock");
-    load_lockfile(&lockfile, pkg, config).chain_error(|| {
-        human(format!("failed to parse lock file at: {}", lockfile.display()))
-    })
-}
+    if !pkg.root().join("Cargo.lock").exists() {
+        return Ok(None)
+    }
 
-pub fn load_lockfile(path: &Path, pkg: &Package, config: &Config)
-                     -> CargoResult<Option<Resolve>> {
-    // If there is no lockfile, return none.
-    let mut f = match File::open(path) {
-        Ok(f) => f,
-        Err(_) => return Ok(None)
-    };
+    let root = Filesystem::new(pkg.root().to_path_buf());
+    let mut f = try!(root.open_ro("Cargo.lock", config, "Cargo.lock file"));
 
     let mut s = String::new();
-    try!(f.read_to_string(&mut s));
-
-    let table = toml::Value::Table(try!(cargo_toml::parse(&s, path)));
-    let mut d = toml::Decoder::new(table);
-    let v: resolver::EncodableResolve = try!(Decodable::decode(&mut d));
-    Ok(Some(try!(v.to_resolve(pkg, config))))
-}
-
-pub fn write_pkg_lockfile(pkg: &Package, resolve: &Resolve) -> CargoResult<()> {
-    let loc = pkg.root().join("Cargo.lock");
-    write_lockfile(&loc, resolve)
+    try!(f.read_to_string(&mut s).chain_error(|| {
+        human(format!("failed to read file: {}", f.path().display()))
+    }));
+
+    (|| {
+        let table = toml::Value::Table(try!(cargo_toml::parse(&s, f.path())));
+        let mut d = toml::Decoder::new(table);
+        let v: resolver::EncodableResolve = try!(Decodable::decode(&mut d));
+        Ok(Some(try!(v.to_resolve(pkg, config))))
+    }).chain_error(|| {
+        human(format!("failed to parse lock file at: {}", f.path().display()))
+    })
 }
 
-pub fn write_lockfile(dst: &Path, resolve: &Resolve) -> CargoResult<()> {
+pub fn write_pkg_lockfile(pkg: &Package,
+                          resolve: &Resolve,
+                          config: &Config) -> CargoResult<()> {
     let mut e = Encoder::new();
     resolve.encode(&mut e).unwrap();
 
@@ -69,20 +63,36 @@ pub fn write_lockfile(dst: &Path, resolve: &Resolve) -> CargoResult<()> {
         None => {}
     }
 
+    let root = Filesystem::new(pkg.root().to_path_buf());
+
     // Load the original lockfile if it exists.
-    if let Ok(orig) = paths::read(dst) {
+    //
+    // If the lockfile contents haven't changed so don't rewrite it. This is
+    // helpful on read-only filesystems.
+    let orig = root.open_ro("Cargo.lock", config, "Cargo.lock file");
+    let orig = orig.and_then(|mut f| {
+        let mut s = String::new();
+        try!(f.read_to_string(&mut s));
+        Ok(s)
+    });
+    if let Ok(orig) = orig {
         if has_crlf_line_endings(&orig) {
             out = out.replace("\n", "\r\n");
         }
         if out == orig {
-            // The lockfile contents haven't changed so don't rewrite it.
-            // This is helpful on read-only filesystems.
             return Ok(())
         }
     }
 
-    try!(paths::write(dst, out.as_bytes()));
-    Ok(())
+    // Ok, if that didn't work just write it out
+    root.open_rw("Cargo.lock", config, "Cargo.lock file").and_then(|mut f| {
+        try!(f.file().set_len(0));
+        try!(f.write_all(out.as_bytes()));
+        Ok(())
+    }).chain_error(|| {
+        human(format!("failed to write {}",
+                      pkg.root().join("Cargo.lock").display()))
+    })
 }
 
 fn has_crlf_line_endings(s: &str) -> bool {

diff --git a/src/cargo/ops/mod.rs b/src/cargo/ops/mod.rs
index b9eb78ac3da3a773f63c50e1491ba6cc7f1d5bd1..be772fe8c04dd8d872af10447faf7a45c769b232 100644 (file)
--- a/src/cargo/ops/mod.rs
+++ b/src/cargo/ops/mod.rs
@@ -13,8 +13,7 @@ pub use self::cargo_doc::{doc, DocOptions};
 pub use self::cargo_generate_lockfile::{generate_lockfile};
 pub use self::cargo_generate_lockfile::{update_lockfile};
 pub use self::cargo_generate_lockfile::UpdateOptions;
-pub use self::lockfile::{load_lockfile, load_pkg_lockfile};
-pub use self::lockfile::{write_lockfile, write_pkg_lockfile};
+pub use self::lockfile::{load_pkg_lockfile, write_pkg_lockfile};
 pub use self::cargo_test::{run_tests, run_benches, TestOptions};
 pub use self::cargo_package::package;
 pub use self::registry::{publish, registry_configuration, RegistryConfig};

diff --git a/src/cargo/ops/resolve.rs b/src/cargo/ops/resolve.rs
index f5925a46c50c9ea1dcbfd183f0213874de57cf35..472027d11a3316daeb0dbc79eb359ef651410238 100644 (file)
--- a/src/cargo/ops/resolve.rs
+++ b/src/cargo/ops/resolve.rs
@@ -20,7 +20,7 @@ pub fn resolve_pkg(registry: &mut PackageRegistry,
                                              Method::Everything,
                                              prev.as_ref(), None));
     if package.package_id().source_id().is_path() {
-        try!(ops::write_pkg_lockfile(package, &resolve));
+        try!(ops::write_pkg_lockfile(package, &resolve, config));
     }
     Ok(resolve)
 }

diff --git a/tests/test_cargo_generate_lockfile.rs b/tests/test_cargo_generate_lockfile.rs
index c8a9bb6a274151312fd709990a906eccec716250..e0231b40c4123c001fff86549ac5679051828181 100644 (file)
--- a/tests/test_cargo_generate_lockfile.rs
+++ b/tests/test_cargo_generate_lockfile.rs
@@ -62,6 +62,7 @@ test!(adding_and_removing_packages {
     assert!(lock2 != lock3);
 
     // remove the dep
+    println!("lock4");
     File::create(&toml).unwrap().write_all(br#"
         [package]
         name = "foo"

diff --git a/tests/tests.rs b/tests/tests.rs
index ecd4ade3320d904da226c3298efba79cb01e6eda..e627cb30c71bfe2e5c504481e61b5ebbdec08d66 100644 (file)
--- a/tests/tests.rs
+++ b/tests/tests.rs
@@ -93,6 +93,7 @@ fn process<T: AsRef<OsStr>>(t: T) -> cargo::util::ProcessBuilder {
     p.cwd(&support::paths::root())
      .env("HOME", &support::paths::home())
      .env_remove("CARGO_HOME")
+     .env_remove("RUSTC")
      .env_remove("XDG_CONFIG_HOME")      // see #2345
      .env("GIT_CONFIG_NOSYSTEM", "1")    // keep trying to sandbox ourselves
      .env_remove("CARGO_TARGET_DIR")     // we assume 'target'